-
Notifications
You must be signed in to change notification settings - Fork 239
Add CUDA version compatibility check #1412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/ok to test 7ce325c |
7ce325c to
1962e35
Compare
|
/ok to test 1962e35 |
|
|
|
||
| - ``CUDA_PYTHON_CUDA_PER_THREAD_DEFAULT_STREAM`` : When set to 1, the default stream is the per-thread default stream. When set to 0, the default stream is the legacy default stream. This defaults to 0, for the legacy default stream. See `Stream Synchronization Behavior <https://docs.nvidia.com/cuda/cuda-runtime-api/stream-sync-behavior.html>`_ for an explanation of the legacy and per-thread default streams. | ||
|
|
||
| - ``CUDA_PYTHON_DISABLE_VERSION_CHECK`` : When set to 1, suppresses the warning that is issued when ``cuda.core`` detects that ``cuda-bindings`` was compiled against a newer CUDA major version than the installed driver supports. This warning helps identify version mismatches that may cause features to not work correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This env var is added to cuda-bindings docs, but the actual change is made to cuda-core? Can we move them to the right place?
Actually, thinking about this we probably should do this check in cuda-bindings instead of cuda-core, since it's what actually matters. The question is if there is a good place that would allow lazy-init. Remember, both cuda-core and cuda-bindings can be installed and imported on CPU-only machines, without driver or a physical GPU installed. I can't think of an obvious solution off top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about cuInit? That seems like a logical place for the version check, though the change might be more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, cuda-bindings provides the check; cuda-core invokes it. Other consumers of cuda-bindings can also invoke the check.
8a0d248 to
73e5a43
Compare
|
/ok to test 73e5a43 |
73e5a43 to
ddd92bd
Compare
|
/ok to test ddd92bd |
rparolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should issue a warning to the user if we can't fetch the driver version.
Add warn_if_cuda_major_version_mismatch() to cuda-bindings that warns when cuda-bindings was compiled for a newer CUDA major version than the installed driver supports. Called by cuda.core on first Device access.
# Conflicts: # cuda_core/pixi.lock
Import warn_if_cuda_major_version_mismatch locally in Device.__new__ after cuInit, using try/except/else pattern instead of module-level import with lambda fallback.
07bf4a4 to
b2083ed
Compare
|
/ok to test b2083ed |
Extract Device.__new__ logic into cdef helper functions: - Device_ensure_cuda_initialized(): cuInit + version check - Device_resolve_device_id(): resolve None to current device or 0 - Device_ensure_tls_devices(): create thread-local singletons Reduces Device.__new__ from ~60 lines to ~12 lines. Helpers placed after Device class following memory module pattern.
a92ed3b to
fdb3a7e
Compare
|
/ok to test fdb3a7e |
|
/ok to test acadb31 |
Integrate RAII handles architecture from main while preserving the version compatibility check feature. Resolved conflict in _device.pyx by keeping the refactored helper functions and updating field names to match RAII object structure.
acadb31 to
3a5c210
Compare
|
/ok to test 3a5c210 |
|
/ok to test 424a113 |
|
/ok to test 6071609 |
leofang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my late review.
|
|
||
| - ``CUDA_PYTHON_CUDA_PER_THREAD_DEFAULT_STREAM`` : When set to 1, the default stream is the per-thread default stream. When set to 0, the default stream is the legacy default stream. This defaults to 0, for the legacy default stream. See `Stream Synchronization Behavior <https://docs.nvidia.com/cuda/cuda-runtime-api/stream-sync-behavior.html>`_ for an explanation of the legacy and per-thread default streams. | ||
|
|
||
| - ``CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING`` : When set to 1, suppresses warnings about CUDA major version mismatches between ``cuda-bindings`` and the installed driver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I am not sure if this env var is really useful, because the added API is entirely opt-in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version check can be triggered indirectly from the end user's POV. For example, by using cuda-core. Without this, each library that calls the version-check function would need its own opt-out, and users might need to set them all. To me it makes sense to place the opt-out alongside the implementation and there is little/no cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I insist that this is useless because this is listed in the cuda-bindings docs, not cuda-core. But cuda-bindings does not call this function anywhere, whereas cuda-core users don't necessarily look at cuda-bindings docs. Can we please create a new environment_variables.rst for cuda-core if we think this is helpful there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref: #1412 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better: Document this env var in both docs.
|
/ok to test 96532b2 |
96532b2 to
973af9b
Compare
|
/ok to test 973af9b |
973af9b to
27732f2
Compare
Replace setup_method/teardown_method with a pytest fixture that uses monkeypatch to properly save and restore the original value of _major_version_compatibility_checked after each test. Minor change to Cython cdef inline helper function signature.
27732f2 to
7f23b08
Compare
|
/ok to test 7f23b08 |
Summary
Adds a version compatibility check that warns users when cuda-bindings was compiled against a newer CUDA major version than the installed driver supports.
Changes
cuda-bindings
check_cuda_version_compatibility()function incuda/bindings/utils/_version_check.pyCUDA_VERSIONvs runtimecuDriverGetVersion()cuda.bindings.utilstests/test_version_check.pycuda-core
Device.__new__callscheck_cuda_version_compatibility()aftercuInitsucceedscuda.bindings.utilsRationale
When cuda-bindings is built against CUDA 13 headers but the user's driver only supports CUDA 12, many features will silently fail or behave unexpectedly. This check provides early, clear feedback:
Design
cuda.bindings.utilssince it checks cuda-bindings' compile-time versionDevicefirst triggers CUDA initializationCUDA_PYTHON_DISABLE_VERSION_CHECK=1to disableFuture Work
We could not find a suitable place to invoke the version check automatically within cuda-bindings itself (e.g., hooking into
cuInit), so the check is currently triggered by cuda-core. This may be revisited in the future.Test Coverage
7 tests in cuda-bindings covering:
Related Work